Fix login - #1160

issue: Guard against undefined givenname when computing fullName to avoid rendering "undefined".

Because userDetails?.givenname !== "" is true when givenname is undefined, the header can show "undefined <sn>". Consider a truthiness check or explicitly handling both undefined and empty string:

const fullName = React.useMemo(() => {
  if (!userDetails) return "";
  if (userDetails.givenname) {
    return `${userDetails.givenname} ${userDetails.sn}`;
  }
  return userDetails.sn;
}, [userDetails]);

Not sure if I understand this...

I've removed the · from each string, instead it's part of the styling of the element, this more follows the common html and css schemantics. The true reason is, that I wanted to insert a link and the PF component only accepts strings...

This is the same as the user parameter you just deleted. But I understand that the new name is more descriptive... Not sure if it make sense to remove the error parameter, just in case the API call returns an error response (can't recall now the chances of that happening).

Scrapped, the user now lives only in the global slice, which makes more sense, as this info was duplicated.

The old code had if (!isInitialBatchLoading && initialBatchResponse === undefined) which properly guarded against transient undefined states. This PR removed the !isInitialBatchLoading guard, causing a briefly flash the login page during the refetch window. Maybe this change can be be reverted?

I've scraped this idea, in favor of fully relying on RTK Query, I'm curious about some checks there...

This useEffect uses isInitialBatchLoading and refetch in its body, but only declares [loggedIn] as a dependency.

I've scraped this idea, all of this is replace by rtk query and immediate caching, instead we refetch whenever we login.

I'm thinking that there is no safeguard here (e.g. error shown) in case the logout operation fails, e.g., due to a failed response, network error, server unreachable, 500, etc. Maybe we should consider to add something here? This can be done in a different PR if needed.

Not sure if this one has been amended? The code seems to be the same...

I see, true, the principle holds, please see the code:

logout().then((response) => {
      if ("data" in response && !response.data?.error) {
        sessionStorage.setItem("isKerberosDisabled", "true");
        dispatch(logoutUser());
      }
    });

In case of non-ok response (anything else but 2xx), the code throws, therefore .then will not run. We even check for errors in ok responses, not sure what else should be added here.

I'm thinking that there is no safeguard here (e.g. error shown) in case the logout operation fails, e.g., due to a failed response, network error, server unreachable, 500, etc. Maybe we should consider to add something here? This can be done in a different PR if needed.

there is not and else branch to this if, Im not sure if thats a problem but it seems odd.

This is correct, if isKerberosDisabled then we don't want to perform auto-login, otherwise we would get stuck in the log in loop.

The check userDetails?.givenname !== "" is true when givenname is undefined (because undefined !== ""), so the header will show "undefined Smith" for users without a first name (e.g., admin). The old code had a proper truthiness check (if (!first)).

Fix suggestion:

const fullName = React.useMemo(() => {
  if (!userDetails) return "";
  if (userDetails.givenname) {
    return `${userDetails.givenname} ${userDetails.sn}`;
  }
  return userDetails.sn;
}, [userDetails]);

Right, I assumed givenname always has to be set, which is true, even administrator has givenname set, to "". Should I still handle this edge case?

Not sure if this one has been amended? The code seems to be the same...